🌱 Add a Makefile target and start running the API diff linter as part of CI #2411
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
camilamacedo86
force-pushed
the
add-ci-api-diff
branch
from
a0b209a to
23d2bf7
Compare
openshift-ci
bot
added
the
do-not-merge/work-in-progress
camilamacedo86
force-pushed
the
add-ci-api-diff
branch
from
23d2bf7 to
a510d69
Compare
camilamacedo86
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2411 +/- ##
==========================================
- Coverage 69.61% 69.58% -0.03%
==========================================
Files 100 100
Lines 7641 7641
==========================================
- Hits 5319 5317 -2
- Misses 1888 1889 +1
- Partials 434 435 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
camilamacedo86
force-pushed
the
add-ci-api-diff
branch
2 times, most recently
from
ea41264 to
1569876
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
camilamacedo86
force-pushed
the
add-ci-api-diff
branch
from
1569876 to
4dbd085
Compare
camilamacedo86
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
camilamacedo86
changed the title
pedjak
left a comment
pedjak
left a comment
There was a problem hiding this comment.
how hard would, i.e. what is the effort to fix all api linter issues in single PR instead of introducing this logic to compare with the baseline?
Very high — and it may never will be done. For example: It fails #2413. I raised for we discuss and based on the discussion it seems that we never will change. Given that, there are many cases we might never want to fix, we will end up saying “this is expected” just to get the PR merged. Then it merges, and in the next round we will be expected to pass again anyway. |
camilamacedo86
force-pushed
the
add-ci-api-diff
branch
from
4dbd085 to
2b86735
Compare
camilamacedo86
changed the title
openshift-ci
bot
added
the
approved
|
@pedjak @grokspawn |
| steps: | ||
| - uses: actions/checkout@v6 | ||
| with: | ||
| fetch-depth: 0 # Fetch all history for all branches (needed for API diff) |
There was a problem hiding this comment.
not sure if this is really needed now, given that we are going to fetch baseline in run.sh if it does not exist anyway?
There was a problem hiding this comment.
Yes, we must to have it to do the diff between main and pr branch.
Without it will not work and will fail
| pull_request: | ||
|
|
||
| jobs: | ||
| lint-api-diff: |
There was a problem hiding this comment.
this job should not execute at all if a PR contains no changes in api folder at all. You can use dorny/paths-filter for that, see https://github.com/operator-framework/operator-controller/blob/main/.github/workflows/files-diff.yaml#L15
There was a problem hiding this comment.
Acknowledged. I intentionally left the step enabled unconditionally to improve CI feedback and make failures easier to catch and address while this stabilizes.
If that’s a merge blocker from your POV, I can change it (it remains runnable locally).
My preference is to keep it always-on for now since it completes in under 2 minutes and simplifies validation.
Then, after we improve it we can change things out when we have fewer issues to get solved
Co-authored-by: Predrag Knezevic <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
By Copilot:
When sourcing .bingo/variables.env, the file contains GOBIN=${GOBIN:=$(go env GOBIN)} which requires the go command to be available. While this works in CI after the setup-go step, the script might fail if run locally without Go in PATH. Consider adding a check that Go is available before sourcing, or handle the source operation with error checking using source .bingo/variables.env 2>/dev/null || true and validate the variable afterward.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pedjak, tmshort The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Co-authored-by: Copilot <[email protected]>
|
/lgtm |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
openshift-merge-bot
bot
merged commit 1fa4169
into
operator-framework:main
4 participants
Description
Reviewer Checklist